18450: feat: support named variables & defaults for `CREATE FUNCTION` by martin-augment · Pull Request #3 · martin-augment/datafusion

martin-augment · 2025-11-06T09:43:15Z

18450: To review by AI

Note

Adds named and default arguments to CREATE FUNCTION with validation and signature generation, supports named ($foo) placeholders, and updates tests/error messages.

CREATE FUNCTION:
- Support named arguments (e.g., $a, $b) and default argument expressions; validate style (all named vs all positional) and disallow non-defaults after defaults.
- Propagate argument names into arg_types and enforce parameter-style consistency.
- Generate multiple signatures for optional/defaulted params using TypeSignature::one_of and store defaults; use defaults during expression simplification when args are omitted.
SQL Parsing/Placeholders:
- Support named placeholders ($foo) alongside positional; for PREPARE, map known names to positional, otherwise error with Unknown placeholder.
UDF Wrapper (ScalarFunctionWrapper):
- Add defaults handling and use it to replace missing args; update simplify replacement logic.
Tests:
- New tests for named and default args in CREATE FUNCTION, covering arity and validation errors.
- Update sqllogictest and params tests to reflect Unknown placeholder errors and new behaviors.

^{Written by Cursor Bugbot for commit 8ddaae0. This will update automatically on new commits. Configure here.}

coderabbitai · 2025-11-06T09:43:29Z

Walkthrough

Implements support for defaulted and named arguments in user-defined scalar functions. Extends ScalarFunctionWrapper to hold defaults, updates placeholder handling to support named placeholders with parameter mapping, and adds validation for function argument consistency and default value ordering.

Changes

Cohort / File(s)	Summary
User-defined scalar functions `datafusion/core/tests/user_defined/user_defined_scalar_functions.rs`	Extends ScalarFunctionWrapper with `defaults: Vec<Option<Expr>>` field to hold per-argument defaults. Updates replacement logic to apply defaults when arguments are missing. Computes composite signatures and defaults from CreateFunction, enabling proper handling of defaulted and named arguments during simplification. Adds tests for named/default argument scenarios and error cases.
Placeholder and parameter handling `datafusion/sql/src/expr/value.rs`	Extends `create_placeholder_expr` to support named placeholders (e.g., $foo) alongside positional ones. Maps named placeholders to parameter positions using param_data_types and rewrites them to positional form. Errors on unknown named placeholders when no match is found.
Function definition validation `datafusion/sql/src/statement.rs`	Adds validation in CreateFunction path: enforces non-default arguments do not follow defaults, and requires consistent argument style (all positional or all named). Derives field names from argument definitions when constructing metadata. Raises early plan errors on validation failures.
Error message updates `datafusion/sql/tests/cases/params.rs`, `datafusion/sqllogictest/test_files/prepare.slt`	Updates error message from "Invalid placeholder, not a number: $foo" to "Unknown placeholder: $foo" for unresolved named placeholders.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch pr-18450-2025-11-06-09-43-11

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d3a25fe and 8ddaae0.

📒 Files selected for processing (5)

datafusion/core/tests/user_defined/user_defined_scalar_functions.rs (7 hunks)
datafusion/sql/src/expr/value.rs (2 hunks)
datafusion/sql/src/statement.rs (2 hunks)
datafusion/sql/tests/cases/params.rs (1 hunks)
datafusion/sqllogictest/test_files/prepare.slt (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

datafusion/sql/src/expr/value.rs (1)

datafusion/expr/src/expr.rs (1)

new_with_field (1171-1173)

datafusion/sql/src/statement.rs (3)

datafusion/sql/src/expr/function.rs (1)

args (787-789)

datafusion/sql/src/relation/mod.rs (1)

args (165-177)

datafusion/sql/src/planner.rs (1)

fields (740-751)

datafusion/core/tests/user_defined/user_defined_scalar_functions.rs (3)

datafusion/expr/src/expr_fn.rs (4)

signature (484-486)

signature (598-600)

signature (691-693)

placeholder (125-130)

datafusion-examples/examples/function_factory.rs (1)

replacement (161-183)

datafusion/expr-common/src/signature.rs (2)

one_of (1162-1168)

exact (1118-1124)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)

GitHub Check: Cursor Bugbot
GitHub Check: Verify MSRV (Min Supported Rust Version)
GitHub Check: Check cargo fmt
GitHub Check: build and run with wasm-pack
GitHub Check: linux build test
GitHub Check: cargo test (macos-aarch64)
GitHub Check: Check License Header
GitHub Check: Verify MSRV (Min Supported Rust Version)
GitHub Check: build and run with wasm-pack
GitHub Check: linux build test
GitHub Check: cargo test (macos-aarch64)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

cursor · 2025-11-06T09:45:38Z

datafusion/core/tests/user_defined/user_defined_scalar_functions.rs

+                                "Function argument {} not provided, argument missing!",
+                                placeholder.id
+                            )?,
+                        }


Bug: Guarded Access to Parameter Defaults

Potential out-of-bounds array access in ScalarFunctionWrapper::replacement(). The code accesses defaults[placeholder_position] without checking if placeholder_position < defaults.len(). If a function body references a placeholder (e.g., $3) that doesn't correspond to a defined parameter, this will panic with an out-of-bounds error. A bounds check should be added before accessing the defaults array.

cursor · 2025-11-06T09:45:38Z

datafusion/core/tests/user_defined/user_defined_scalar_functions.rs

+        .await
+        .expect_err("cannot mix named and positional style");
+    let expected = "Error during planning: All function arguments must use either named or positional style.";
+    assert!(expected.starts_with(&err.strip_backtrace()));


Bug: Swapped Prefix Check for Error Messages

The arguments to starts_with() are reversed. The code checks expected.starts_with(&err.strip_backtrace()), which checks if the expected error message starts with the actual error. This should be err.strip_backtrace().starts_with(expected) to check if the actual error starts with the expected prefix, or use assert_eq! for exact comparison.

cursor · 2025-11-06T09:45:39Z

datafusion/core/tests/user_defined/user_defined_scalar_functions.rs

+        .expect_err("non-default argument cannot follow default argument");
+    let expected =
+        "Error during planning: Non-default arguments cannot follow default arguments.";
+    assert!(expected.starts_with(&err.strip_backtrace()));


Bug: Reversed prefixes in starts_with assertion

The arguments to starts_with() are reversed. The code checks expected.starts_with(&err.strip_backtrace()), which checks if the expected error message starts with the actual error. This should be err.strip_backtrace().starts_with(expected) to check if the actual error starts with the expected prefix, or use assert_eq! for exact comparison.

augmentcode

Review completed. No suggestions at this time.

r1b added 4 commits November 5, 2025 17:26

feat: support named arguments, defaults in udfs

325110a

improve error message on unknown placeholder

6be0061

update docstring / comment in create_placeholder_expr

83fc06b

fix tests, default argument validation

8ddaae0

cursor bot reviewed Nov 6, 2025

View reviewed changes

augmentcode bot reviewed Nov 6, 2025

View reviewed changes

martin-augment closed this Nov 6, 2025

claude bot mentioned this pull request Dec 2, 2025

19042: Add sorted data benchmark. #94

Open

claude bot mentioned this pull request Dec 17, 2025

19369: Feat: Allow pow with negative & non-integer exponent on decimals #130

Open

claude bot mentioned this pull request Jan 2, 2026

19597: Refactor cache APIs to support ordering information #160

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

18450: feat: support named variables & defaults for `CREATE FUNCTION`#3

18450: feat: support named variables & defaults for `CREATE FUNCTION`#3
martin-augment wants to merge 4 commits intomainfrom
pr-18450-2025-11-06-09-43-11

martin-augment commented Nov 6, 2025 •

edited by cursor bot

Loading

Uh oh!

coderabbitai bot commented Nov 6, 2025 •

edited

Loading

Uh oh!

cursor bot Nov 6, 2025

Uh oh!

cursor bot Nov 6, 2025

Uh oh!

cursor bot Nov 6, 2025

Uh oh!

augmentcode bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

martin-augment commented Nov 6, 2025 • edited by cursor bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

cursor bot Nov 6, 2025

Choose a reason for hiding this comment

Bug: Guarded Access to Parameter Defaults

Uh oh!

cursor bot Nov 6, 2025

Choose a reason for hiding this comment

Bug: Swapped Prefix Check for Error Messages

Uh oh!

cursor bot Nov 6, 2025

Choose a reason for hiding this comment

Bug: Reversed prefixes in starts_with assertion

Uh oh!

augmentcode bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

martin-augment commented Nov 6, 2025 •

edited by cursor bot

Loading

coderabbitai bot commented Nov 6, 2025 •

edited

Loading